Add default trait implementations for "c-unwind" ABI function pointers #101263

lopopolo · 2022-09-01T05:21:46Z

Following up on #92964, only add default trait implementations for the c-unwind family of function pointers. The previous attempt in #92964 added trait implementations for many more ABIs and ran into concerns regarding the increase in size of the libcore rlib.

An attempt to abstract away function pointer types behind a unified trait to reduce the duplication of trait impls is being discussed in #99531 but this change looks to be blocked on a lang MCP.

Following @RalfJung's suggestion in #99531 (comment), this commit is another cut at #92964 but it only adds the impls for extern "C-unwind" fn and unsafe extern "C-unwind" fn.

I am interested in landing this patch to unblock the stabilization of the c_unwind feature.

RFC: rust-lang/rfcs#2945
Tracking Issue: #74990

rust-highfive · 2022-09-01T05:21:50Z

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

lopopolo · 2022-09-01T05:40:03Z

it looks like I need to bless changes to the UI tests. Let me try and figure out how to do that 😄

Edit: done!

RalfJung · 2022-09-01T07:44:36Z

@bors try @rust-timer queue

rust-timer · 2022-09-01T07:44:38Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-09-01T07:44:45Z

⌛ Trying commit 2f7cc2d574b203e1f19b63ee705fe217fefc31ee with merge c97600b8b2ec5ef84e1426b75f8be57bf5450712...

bors · 2022-09-01T09:21:46Z

☀️ Try build successful - checks-actions
Build commit: c97600b8b2ec5ef84e1426b75f8be57bf5450712 (c97600b8b2ec5ef84e1426b75f8be57bf5450712)

rust-timer · 2022-09-01T09:21:48Z

Queued c97600b8b2ec5ef84e1426b75f8be57bf5450712 with parent aa857eb, future comparison URL.

rust-timer · 2022-09-01T13:26:42Z

Finished benchmarking commit (c97600b8b2ec5ef84e1426b75f8be57bf5450712): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	1.0%	[0.3%, 2.6%]	14
Regressions ❌ (secondary)	1.9%	[0.6%, 2.7%]	22
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.1%	[-1.4%, -0.8%]	6
All ❌✅ (primary)	1.0%	[0.3%, 2.6%]	14

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	1.4%	[1.4%, 1.4%]	1
Regressions ❌ (secondary)	2.9%	[1.5%, 4.3%]	9
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.4%	[1.4%, 1.4%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	2.3%	[2.0%, 2.8%]	3
Regressions ❌ (secondary)	2.8%	[2.2%, 4.5%]	10
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.3%	[2.0%, 2.8%]	3

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

RalfJung · 2022-09-01T15:44:34Z

Yeah, as expected that extra metadata in the rlib makes everything measurably slower. Not sure if there is a way that we can avoid some of these instances (do we really need all the traits here), but in the end it boils down to judging whether that's a cost we are willing to accept for the benefit of #74990.

Sounds like a @rust-lang/compiler question to me.

pnkfelix · 2022-09-08T15:12:44Z

Discussed in T-compiler triage meeting.

I think we should move forward with this, despite the performance hit. I don't want to block c-unwind based on the performance regression observed above.

(If it turns out that the performance impact matters, then we can adopt other approaches, such as the one suggested in #99531; though I'm hoping that will not be necessary solely for this issue.)

@rustbot label: -I-compiler-nominated

pnkfelix · 2022-09-08T15:13:52Z

(also, as a side note: the primary benchmarks only regressed on "doc full" scenarios. Normal compilation (check/debug/opt) all came out looking great I would say.)

@rustbot label: +perf-regression-triaged

lopopolo · 2022-09-08T16:31:10Z

@RalfJung Given the above triage on the perf regression from the compiler team, what are the next steps for getting this approved and merged?

RalfJung · 2022-09-08T16:43:52Z

It needs someone from t-libs to review and approve.
r? libs

thomcc

Impl looks fine, I think this needs libs-api signoff, or perhaps an ACP.

@RalfJung

Following up on #92964, only add default trait implementations for the `c-unwind` family of function pointers. The previous attempt in #92964 added trait implementations for many more ABIs and ran into concerns regarding the increase in size of the libcore rlib. An attempt to abstract away function pointer types behind a unified trait to reduce the duplication of trait impls is being discussed in #99531 but this change looks to be blocked on a lang MCP. Following @RalfJung's suggestion in #99531 (comment), this commit is another cut at #92964 but it _only_ adds the impls for `extern "C-unwind" fn` and `unsafe extern "C-unwind" fn`. I am interested in landing this patch to unblock the stabilization of the `c_unwind` feature. RFC: rust-lang/rfcs#2945 Tracking Issue: #74990

lopopolo · 2022-10-20T02:42:16Z

I couldn't figure out the right combination of --stage and --keep-stage flags to pass to x.py to check this locally, but I did rebase to include #103239 and reworked the macro from my proposed patch above. Let's see if CI passes.

lopopolo · 2022-10-20T02:48:50Z

Looks like the build still fails with an #[unstable] annotation here has no effect error. @m-ou-se do I need to wait for a new bootstrap compiler to be released?

m-ou-se · 2022-10-20T10:20:26Z

Looks like the build still fails with an #[unstable] annotation here has no effect error. @m-ou-se do I need to wait for a new bootstrap compiler to be released?

Oh. :( Yeah. I suppose you could apply #[cfg(not(bootstrap))] to line 1868. These trait impls aren't used in the compiler, meaning it's fine if they're missing from the first stage std.

These need to wait until #103239 makes it into the bootstrap compiler.

lopopolo · 2022-10-20T15:59:52Z

@thomcc I've made the stability annotation changes. This is ready for review.

@rustbot ready

thomcc · 2022-10-20T16:23:34Z

LGTM, thanks.

@bors r+

bors · 2022-10-20T16:23:36Z

📌 Commit efe61da has been approved by thomcc

It is now in the queue for this repository.

bors · 2022-10-21T20:59:06Z

⌛ Testing commit efe61da with merge 5c8bff7...

bors · 2022-10-21T23:41:54Z

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing 5c8bff7 to master...

rust-timer · 2022-10-22T01:00:26Z

Finished benchmarking commit (5c8bff7): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	0.9%	[0.2%, 2.6%]	19
Regressions ❌ (secondary)	1.9%	[0.4%, 3.0%]	23
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.9%	[0.2%, 2.6%]	19

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	1.4%	[1.4%, 1.4%]	1
Regressions ❌ (secondary)	2.8%	[1.5%, 4.9%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.4%	[-1.4%, -1.4%]	1
All ❌✅ (primary)	1.4%	[1.4%, 1.4%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	2.5%	[2.1%, 3.0%]	2
Regressions ❌ (secondary)	2.7%	[2.1%, 3.1%]	13
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.5%	[2.1%, 3.0%]	2

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

nnethercote · 2022-10-22T10:01:26Z

It's all doc perf regressions, and was decided earlier to be acceptable.

@rustbot label: +perf-regression-triaged

rust-highfive assigned m-ou-se Sep 1, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 1, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 1, 2022

This was referenced Sep 1, 2022

builtin trait to abstract over function pointers #99531

Closed

Tracking Issue for "C-unwind ABI", RFC 2945 #74990

Closed

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 1, 2022

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 1, 2022

RalfJung added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-compiler-nominated Nominated for discussion during a compiler team meeting. labels Sep 1, 2022

rustbot removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Sep 8, 2022

rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 8, 2022

rust-highfive assigned thomcc and unassigned m-ou-se Sep 8, 2022

thomcc approved these changes Sep 9, 2022

View reviewed changes

thomcc removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 9, 2022

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2022

lopopolo added 3 commits October 19, 2022 19:17

Bless ui tests

d0a33f2

Update stability annotations on fnptr impls for C-unwind ABI

5316796

rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Oct 20, 2022

This comment was marked as outdated.

Sign in to view

This comment has been minimized.

Sign in to view

Skip C-unwind fn pointer impls with the bootstrap compiler

efe61da

These need to wait until #103239 makes it into the bootstrap compiler.

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 20, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 20, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 21, 2022

bors merged commit 5c8bff7 into rust-lang:master Oct 21, 2022

rustbot added this to the 1.66.0 milestone Oct 21, 2022

lopopolo deleted the lopopolo/c-unwind-fn-ptr-impls branch October 21, 2022 23:47

This was referenced Oct 22, 2022

Support for generating bindings with the C-unwind ABI rust-lang/rust-bindgen#2224

Closed

Implement mrb_raise with Rust panic! artichoke/artichoke#1904

Open

lopopolo mentioned this pull request Oct 25, 2022

RFC: 'C-unwind' ABI rust-lang/rfcs#2945

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default trait implementations for "c-unwind" ABI function pointers #101263

Add default trait implementations for "c-unwind" ABI function pointers #101263

lopopolo commented Sep 1, 2022

rust-highfive commented Sep 1, 2022

lopopolo commented Sep 1, 2022 •

edited

Loading

RalfJung commented Sep 1, 2022

rust-timer commented Sep 1, 2022

bors commented Sep 1, 2022

bors commented Sep 1, 2022

rust-timer commented Sep 1, 2022

rust-timer commented Sep 1, 2022

RalfJung commented Sep 1, 2022

pnkfelix commented Sep 8, 2022

pnkfelix commented Sep 8, 2022

lopopolo commented Sep 8, 2022

RalfJung commented Sep 8, 2022

thomcc left a comment

This comment was marked as outdated.

lopopolo commented Oct 20, 2022

lopopolo commented Oct 20, 2022 •

edited

Loading

This comment has been minimized.

m-ou-se commented Oct 20, 2022 •

edited

Loading

lopopolo commented Oct 20, 2022

thomcc commented Oct 20, 2022

bors commented Oct 20, 2022

bors commented Oct 21, 2022

bors commented Oct 21, 2022

rust-timer commented Oct 22, 2022

nnethercote commented Oct 22, 2022

Add default trait implementations for "c-unwind" ABI function pointers #101263

Add default trait implementations for "c-unwind" ABI function pointers #101263

Conversation

lopopolo commented Sep 1, 2022

rust-highfive commented Sep 1, 2022

lopopolo commented Sep 1, 2022 • edited Loading

RalfJung commented Sep 1, 2022

rust-timer commented Sep 1, 2022

bors commented Sep 1, 2022

bors commented Sep 1, 2022

rust-timer commented Sep 1, 2022

rust-timer commented Sep 1, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

RalfJung commented Sep 1, 2022

pnkfelix commented Sep 8, 2022

pnkfelix commented Sep 8, 2022

lopopolo commented Sep 8, 2022

RalfJung commented Sep 8, 2022

thomcc left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

lopopolo commented Oct 20, 2022

lopopolo commented Oct 20, 2022 • edited Loading

This comment has been minimized.

m-ou-se commented Oct 20, 2022 • edited Loading

lopopolo commented Oct 20, 2022

thomcc commented Oct 20, 2022

bors commented Oct 20, 2022

bors commented Oct 21, 2022

bors commented Oct 21, 2022

rust-timer commented Oct 22, 2022

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

nnethercote commented Oct 22, 2022

lopopolo commented Sep 1, 2022 •

edited

Loading

lopopolo commented Oct 20, 2022 •

edited

Loading

m-ou-se commented Oct 20, 2022 •

edited

Loading